🤖 Kimi Code Review

Overall: This is a well-architected standalone monitoring tool with clear separation of concerns, good documentation (CONTRACT.md), and robust error handling. The code is idiomatic Rust and the frontend uses efficient canvas rendering. Only one critical issue blocks compilation.

1. Critical: Invalid dependency version

File: tooling/event-monitor/Cargo.toml
Line: 25

toml = "1.1.3"

Issue: The toml crate has no version 1.1.3 (latest is 0.8.19). This will cause cargo to fail resolving the dependency.

Fix:

toml = "0.8"

2. Code correctness & robustness

File: tooling/event-monitor/src/collector.rs
Line: ~166 (inside connect_and_stream)

let url = format!(
    "{}/lean/v0/events?topics={}",
    node.url.trim_end_matches('/'),
    topics.join(",")
);

Issue: Topics are not URL-encoded. If a topic name contains reserved characters (e.g., &, =, %), the query string will be malformed.

Fix: Use reqwest::Url or percent-encode:

let url = format!("{}/lean/v0/events", node.url.trim_end_matches('/'));
let url = reqwest::Url::parse_with_params(&url, &[("topics", topics.join(","))])?;

File: tooling/event-monitor/src/main.rs
Line: ~69

axum::serve(listener, app).await?;

Issue: No graceful shutdown handling. The server will not respond to SIGINT/SIGTERM cleanly, potentially dropping active SSE connections.

Fix: Add signal handling:

let server = axum::serve(listener, app);
let shutdown = tokio::signal::ctrl_c();
tokio::select! {
    result = server => result?,
    _ = shutdown => tracing::info!("shutting down"),
}

File: tooling/event-monitor/src/config.rs
Line: ~42 (Config::load)

Issue: No validation that nodes is non-empty or that URLs are valid. An empty node list results in a useless process; invalid URLs will fail later at runtime.

Fix: Add validation after loading:

if config.nodes.is_empty() {
    return Err(anyhow::anyhow!("config must specify at least one node"));
}

File: tooling/event-monitor/src/server.rs
Line: ~56

let serve_dir = ServeDir::new(static_dir).fallback(ServeFile::new(index_html));

Issue: static_dir comes from user config without path sanitization. While ServeDir is generally safe, ensure static_dir is resolved to an absolute path to avoid ambiguity.

Fix: Resolve to absolute path:

let static_dir = std::fs::canonicalize(static_dir)?;

3. Security considerations

File: tooling/event-monitor/src/server.rs
Issue: No CORS configuration. If the dashboard is accessed from a different origin (e.g., during development or if behind a reverse proxy), API calls will fail.

Fix: Add tower_http::cors::CorsLayer (optional but recommended):

use tower_http::cors::CorsLayer;
// ...
Router::new()
    .layer(CorsLayer::permissive()) // or restricted to specific origins

4. Performance & correctness observations

File: tooling/event-monitor/src/model.rs
Line: ~182 (aggregate_id)

let canonical = serde_json::to_string(&input).expect("aggregate id input is always valid JSON");

Observation: The expect is safe here since the input is a controlled struct with no floats or maps, but document this invariant.

File: tooling/event-monitor/src/collector.rs
Line: ~236 (now_ms)

i64::try_from(duration.as_millis()).unwrap_or(i64::MAX)

Good: Correctly handles the theoretical overflow case.

File: tooling/event-monitor/src/hub.rs
Line: 20

const HISTORY_MAX_EVENTS: usize = 50_000;

Good: Hard cap prevents unbounded memory growth during attestation floods.

File: tooling/event-monitor/web/app.js
Line: ~43 (applyStatus)

rate.textContent = `${status.events_per_sec.toFixed(1)}/s`;

Issue: If the backend sends NaN or Infinity (possible if the rate tracker window is zero), this displays poorly.

Fix: Add a guard:

if (Number.isFinite(status.events_per_sec)) {
    rate.textContent = `${status.events_per_sec.toFixed(1)}/s`;
} else {
    rate.textContent = "—";
}

5. Testing

File: tooling/event-monitor/tests/sse_integration.rs

Good: Integration test with a fake SSE server is well-written and covers the normalization pipeline end-to-end.

Suggestion: Add a test for the pruning logic in hub.rs to verify the HISTORY_MAX_EVENTS cap is respected under load.

Summary of required changes

1. Fix Cargo.toml toml version to 0.8.
2. Fix URL construction in collector.rs to encode query parameters.
3. Add graceful shutdown handling in main.rs.
4. Validate config has at least one node.

The rest are suggestions for hardening. The architecture is sound and the separation between the Rust collector and JS frontend via CONTRACT.md is excellent practice.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

🤖 Codex Code Review

1. tooling/event-monitor/src/collector.rs: the SSE dial path has no connect/header timeout. client.get(&url).send().await can block indefinitely on a blackholed or half-open node, which means that collector task never reaches the retry/backoff path and may sit in reconnecting forever instead of transitioning to down. For a long-lived SSE stream you still want an initial connect timeout, even if you do not want a read timeout after the stream is established.

2. tooling/event-monitor/src/collector.rs: a clean EOF resets the backoff (Ok(()) => backoff.reset()), then the loop sleeps only the initial 250ms again. If an upstream/proxy closes the SSE stream immediately or periodically, this becomes a tight reconnect loop that never ramps to MAX_BACKOFF and never reports down, which is both noisy and needlessly aggressive toward the node. EOF should be treated as a failed attempt unless you intentionally want “server closed stream” to count as healthy.

3. tooling/event-monitor/src/hub.rs: history pruning only removes stale events from the front of the deque. That assumes near-monotonic slot order, but collectors across nodes/topics can deliver late older events. If a stale slot arrives after newer ones, it gets appended to the back and survives in /api/history past history_slots, so a newly opened dashboard can be seeded with out-of-window data. This needs a full scan/retain, or storage ordered by slot rather than arrival.

No consensus-path code is touched here, so fork choice, attestation validation, STF, XMSS, and SSZ correctness are not directly affected by this PR.

I couldn’t run cargo test in this sandbox because rustup tried to write under a read-only home directory, so the review is from static inspection. Aside from the points above, the normalization logic and the overall separation between collector, hub, and server are straightforward and maintainable.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

🤖 Claude Code Review

Review: PR 538 — tooling/event-monitor

This is a standalone, well-isolated tool (own Cargo workspace, zero deps on ethlambda crates) with a frozen CONTRACT.md interface, strong test coverage (29 tests spanning unit + a real in-process SSE integration test), and careful defensive coding. Overall quality is high; findings below are minor.

Strengths worth calling out:

• No panics on the hot path: unknown topics/malformed JSON are Err-mapped and logged, never unwrap()'d; the one genuine expect() (serializing an internal struct with no maps/floats) is justified with a comment explaining why it's infallible.
• hub.rs's history ring bounds memory two ways (slot-age prune + hard HISTORY_MAX_EVENTS cap) — good protection against an attestation flood before the slot-based prune catches up.
• Frontend (app.js, beeswarm.js, propagation.js) consistently uses textContent/createElement instead of innerHTML for anything derived from server data — no XSS surface despite rendering arbitrary node names/topics from the wire.
• Static file serving uses tower_http::ServeDir/ServeFile, which handles path traversal correctly rather than hand-rolling path joins.
• aggregate_id deliberately serializes a typed struct (not an arbitrary serde_json::Value) to get deterministic field ordering for the content hash — a subtle correctness point that's called out in the comment.
• The startup backfill logic in app.js (buffer live events while /api/history loads, then flush + de-dup by composite key) correctly avoids a gap between "stream opened" and "history fetched" without double-counting.
• NormalizedEvent mapping in model.rs matches CONTRACT.md §3 exactly for every topic variant, including the aggregate id derivation and the attestation → id: null rule.

Minor items (reported via findings tool):

1. timing::bootstrap always does a network round-trip to fetch genesis/spec even when the config fully overrides both genesis_time and ms_per_slot — wastes up to FETCH_TIMEOUT per configured node on every offline/fully-overridden startup.
2. The SSE URL in collector::connect_and_stream is built via format! without percent-encoding node.url/topics — low risk since these come from trusted local config, not attacker input, but a malformed value (stray space/&) would silently produce a bad URL rather than fail config validation.

Nothing here rises to a correctness or security blocker — this reads as a mature, ready-to-merge dev-tooling addition.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Greptile Summary

This PR adds a standalone dashboard for monitoring event arrival and propagation times. The main changes are:

• Rust collectors for multiple node SSE streams.
• Bounded history backfill and live status updates.
• Browser beeswarm views for slot offsets and propagation delays.
• Configuration, documentation, tests, and an offline demo mode.

Confidence Score: 5/5

No additional blocking issue qualifies for a new follow-up comment.

• The remaining configuration failure is already covered by the existing feedback.
• The out-of-order history behavior is part of the existing history-pruning issue.
• No separate production failure requiring an independent change was found.

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into feat/event-moni..." | Re-trigger Greptile}

🤖 Codex Code Review

1. ms_per_slot/intervals_per_slot are never validated, so a bad spec response or config typo can wedge the dashboard. bootstrap() accepts zero values from /lean/v0/config/spec or overrides unchanged, and both canvases later do const step = msPerSlot / intervals; for (let ms = 0; ...; ms += step) in beeswarm.js and propagation.js. ms_per_slot = 0 makes step == 0, so the render loop never advances and the tab hangs. Reject zero/invalid timing inputs at config/bootstrap time.

2. Static-file fallback is too broad and turns missing assets/API typos into 200 index.html responses. build_router() uses ServeDir::fallback(ServeFile::new(index_html)), so /app.jss, /favicon.ico, or even an unhandled /api/... path will return HTML instead of a 404. That makes broken asset paths fail opaquely in the browser and hides server-side routing mistakes. Route / explicitly to index.html and let other unknown paths 404.

3. Startup backfill can temporarily roll node status backward. In app.js, live status events are applied immediately, but during backfill the later history.status.forEach(...) can overwrite a newer live status with an older snapshot. The chip self-heals on the next heartbeat, but the UI can show stale connected/reconnecting/down state or an older rate for several seconds. Buffer/de-dup status during backfill too, or add a monotonic timestamp/sequence.

No consensus-layer logic is touched in this PR, so fork choice, attestation validation, STF, XMSS, and SSZ correctness are unaffected here.

I couldn’t run cargo test in this sandbox: the standalone crate needs crates.io access and the environment has network disabled with no cached dependencies.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

🤖 Claude Code Review

I have findings to report, but let me give the full written review directly since the task calls for markdown output, not the structured tool.

Review: feat(tooling): add event-monitor arrival-time dashboard (PR #538)

Overall this is a well-scoped, cleanly isolated addition. It's a standalone Cargo workspace with zero ethlambda-* dependencies, a frozen CONTRACT.md interface, solid unit/integration test coverage (backoff, rate tracking, normalization edge cases, history pruning, SSE integration against a real in-process server), and the "never panic on unparsable input" invariant is followed consistently in model::normalize and collector::handle_frame. Since this doesn't touch consensus logic, fork choice, or the STF, the security/correctness bar is lower than for core crates, but a few things are worth tightening:

Minor

1. Timing::offset_ms can overflow on adversarial/corrupted slot values (src/timing.rs:41)
   slot as i64 * self.ms_per_slot as i64 is an unchecked multiplication over a slot value that comes straight from an upstream node's SSE payload (model.rs deserializes it as a bare u64, no bounds check). A node that's buggy, malicious, or just sends a stray huge slot (e.g. near u64::MAX) can make this multiplication overflow. In cargo run/cargo test (debug profile) that's a panic; in --release it silently wraps to a bogus offset. Given the codebase's stated philosophy elsewhere ("never panics: an unknown topic or payload we can't parse is logged and dropped") it'd be consistent to use saturating_mul/checked_mul here too, since this is the one place raw slot reaches arithmetic rather than just being echoed back.

2. No timeout on the initial SSE connect request (src/collector.rs:152)
   client.get(&url).send().await? has no per-request timeout (unlike fetch_json in timing.rs, which sets FETCH_TIMEOUT). If a node accepts the TCP connection but never sends response headers, that collector task blocks forever on send(), and the node's status stays stuck at Reconnecting indefinitely rather than ever settling to Down — even though the backoff/status contract (CONTRACT.md §4) implies a bounded transition. Doesn't affect other nodes (each runs as its own spawned task), but worth adding a connect/response timeout for consistency with the rest of the retry/status machinery.

Nits (no action needed, just flagging for awareness)

• History::prune() (src/hub.rs:75) assumes events arrive at the front of the deque in roughly increasing slot order to bound memory by slot age; an event for a stale slot appended out of order at the back would only be evicted via the hard HISTORY_MAX_EVENTS cap, not the slot-age prune. The code comment already calls this out explicitly as an accepted approximation, so this is just confirming that tradeoff is understood, not a new finding.
• aggregate_id's FNV-1a-64 hash is explicitly documented as session-stable/non-cryptographic and used only for UI grouping — appropriate for the use case, no concern.

Things done well

• CONTRACT.md is genuinely kept authoritative — I checked the §2 topic table, §3 NormalizedEvent shape, and §6 propagation-toggle spec against the actual Rust/JS code and they match, including the most recent commit that dropped head/safe_target/chain_reorg from the propagation panel.
• Reconnect backoff, rate tracking, and history pruning all have direct unit tests, not just integration smoke tests.
• bootstrap() fetching genesis + spec concurrently via tokio::join! (timing.rs:94) is a nice touch so a dead node only costs one timeout instead of two serial ones.
• Frontend correctly avoids innerHTML injection risk for node-supplied strings — uses textContent for user/node-derived text throughout app.js/beeswarm.js/propagation.js.
• Startup backfill ordering (open /stream first, buffer, then fetch /api/history, then de-dup and flush) is correctly reasoned to avoid both gaps and double-counting.

No blocking issues — the two minor points above are defensive-coding suggestions for a tool that ingests data from network-facing (if trusted) nodes, not correctness bugs in the current happy path.

Automated review by Claude (Anthropic) · sonnet · custom prompt